Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Ingress Nginx version #565

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Update Ingress Nginx version #565

merged 1 commit into from
Feb 23, 2022

Conversation

phillebaba
Copy link
Contributor

This looks like a major change from the outside but hopefully is not one. The main difference is dropping support for networking.k8s.io/v1beta and switching to networking.k8s.io/v1. The consequence of this is dropping support for clusters older than 1.19, which we are not running anyway.

We are going to have to test the upgrade before merging this just to be sure. I have also done some reading in the Helm chart and project changelogs to see if there are any obvious issues.
https://github.com/kubernetes/ingress-nginx/blob/main/Changelog.md
https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/CHANGELOG.md

One issue is the change from ingress class annotations to the IngressClass resource. We are already create an IngressClass resource but do not enforce the use of a specific default class. This has to be verified that the controller still works without an ingress class. Additionally it seems like the permissions changed to be cluster wide which has caused issues for some people.
kubernetes/ingress-nginx#7578

There seems to be a change in the Helm chart which adds an option for internal/external load balancers. I do not know how that will affect our current method of configuring this.
kubernetes/ingress-nginx#7806

The default annotation word block list has been changed to an empty string. Not really sure what the current value is in our version but this has to be verified to work in the places we use lua. We should probably just follow the documentation recommendations.
https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#annotation-value-word-blocklist

@phillebaba phillebaba mentioned this pull request Feb 22, 2022
6 tasks
@phillebaba
Copy link
Contributor Author

Merging this is not contingent on upgrading to 1.22. It would be best to just get this upgrade out if it works.

@NissesSenap
Copy link
Contributor

We already use the default annotation word block list by default so this shouldn't be a issue for us.

Can the thing of internal/external load balancers have any impact on our public/private ingress config?

The PR it self LGTM other then a missing changelog :)

@phillebaba phillebaba force-pushed the update/ingress-nginx branch 2 times, most recently from 8a6d2b2 to 5e0b7cf Compare February 23, 2022 11:36
@phillebaba
Copy link
Contributor Author

Done my testing and it seems like there is only a single issue. That being the new ingress class requirement. Each ingress is now required to have an ingress class set unless we opt in to watch ingress resources without an ingress class. I prefer that we do not do this opt in.

I have now configured so that there will always exist a default ingress class. It will either be nginx or nginx-public depending on the setup. New Ingress resources will receive the default ingress class, so developers do not have to change anything. Existing Ingress resources have to be patched with the default ingress class.

For me this is the way forward. That we consider this change breaking with the requirement that all Ingress resources have to be patched before applying.

More info can be found here.
https://kubernetes.github.io/ingress-nginx/#i-have-only-one-ingress-controller-in-my-cluster-what-should-i-do

@phillebaba
Copy link
Contributor Author

phillebaba commented Feb 23, 2022

The following script can be used to patch all ingress resources in a cluster with a ingress class.

for ns in $(kubectl get namespace --output=jsonpath={.items..metadata.name}); do
  for ing in $(kubectl -n $ns get ingress --output=jsonpath={.items..metadata.name}); do
    kubectl -n $ns patch ingress $ing -p '{"spec":{"ingressClassName": "nginx"}}'
  done
done

Replace nginx with nginx-public if setup uses public/private ingress controllers.

Adding --dry-run="server" after the patch command will allow for a dry run to verify the changes.

Copy link
Contributor

@NissesSenap NissesSenap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@phillebaba phillebaba merged commit 14fe7e1 into main Feb 23, 2022
@phillebaba phillebaba deleted the update/ingress-nginx branch February 23, 2022 12:57
@NissesSenap
Copy link
Contributor

helm install fails if you have multiple ingress-controllers (public and private).
This is due to the IngressClass.spec.controller fields is immutable.

The workaround to solve this is to delete the ingressClass temporary and run terraform again.

failed ingress-nginx-4.0.17 1.1.1 Upgrade "ingress-nginx-public" failed: cannot patch "nginx-public" with kind IngressClass: IngressClass.networking.k8s.io "nginx-public" is invalid: spec.controller: Invalid value: "k8s.io/ingress-nginx-public": field is immutable

But when updating the ingress-nginx I got downtime, probably because of the old ingresses not being able to get the new requests. So think about this when updating to this version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants